[RSDK-11703, RSDK-11602] Update C++ GetImages signature #481
Conversation
hexbabe
requested review from
dhritinaidu and
njooma
and removed request for
a team
hexbabe
force-pushed
the
RSDK-11703-and-11602
branch
2 times, most recently
from
8b56b94 to
955d8b0
Compare
hexbabe
changed the title
|
@lia-viam @njooma @dhritinaidu manual testing is now done. Please review |
| .with(extra, | ||
| [&](auto& request) { | ||
| for (const auto& source_name : filter_source_names) { | ||
| *request.add_filter_source_names() = source_name; |
There was a problem hiding this comment.
does request have a mutable_filter_source_names() member or similar? if so we could do *request.mutable_filter_source_names() = filter_source_names() rather than copying elementwise
There was a problem hiding this comment.
ah sorry so what I meant here was that we could maybe replace the entirety for the for loop with just *request.mutable_filter_source_names() = std::move(filter_source_names)
There was a problem hiding this comment.
my bad. Tried to address
There was a problem hiding this comment.
shoot looks like it's failing in CI because of the old grpc version. you can just revert to what you had before, i forgot about stupid RepeatedPtrField
There was a problem hiding this comment.
ah gotcha, by "what you had before" do you mean the element-wise copying, or the for loop with the mutable_filter_source_names?
There was a problem hiding this comment.
i think you were doing element-wise copying in both of them so whichever makes the build pass is fine! for reference here's how we do it for vectors of proto types
https://github.com/viamrobotics/viam-cpp-sdk/blob/main/src/viam/sdk/common/private/repeated_ptr_convert.hpp#L16
There was a problem hiding this comment.
got it. Does the comment I added make sense?
There was a problem hiding this comment.
ah not quite, you can omit the comment or just write
// in newer gRPC versions we would be able to call `Add` or `Assign` on an iterator range rather than element-wise copy
b1952dd to
5a5f9bf
Compare
hexbabe
force-pushed
the
RSDK-11703-and-11602
branch
from
5a5f9bf to
a23a644
Compare
hexbabe
force-pushed
the
RSDK-11703-and-11602
branch
from
a23a644 to
d16c6c7
Compare
|
thanks for review! |
2 participants
This PR is part of the SDK changes of the Consolidate GetImage(s) project
It
Manual Testing
Built a local RealSense module executable with its C++ SDK pinned to this commit d8292df
Added logging to make sure that the server is receiving extra and filter_source_names. It was
Verified client received back metadata and loaded the image properly.